feat: add frontend API timeout and cancellation support - #286
feat: add frontend API timeout and cancellation support#286SHAURYAKSHARMA24 wants to merge 2 commits into
Conversation
TFT444
left a comment
TFT444
left a comment
There was a problem hiding this comment.
Clean, scoped fix with no unrelated changes pulled in. CVE and Dependabot alert are properly referenced and the updated lockfile looks correct. Good to go. Approving.
@ritiksah141 @parthrohit22 please have look into that thanks
TFT444
left a comment
TFT444
left a comment
There was a problem hiding this comment.
Went through the abort logic carefully. The abortSource preference ordering handles the caller-vs-timeout race correctly and the inFlight guard prevents stale state on rapid re-renders. Tests are focused and meaningful. Approving.
parthrohit22
left a comment
parthrohit22
left a comment
There was a problem hiding this comment.
The core apiFetch timeout/abort implementation is genuinely well-built: clearTimeout and the caller-signal listener removal both run unconditionally in a finally covering every exit path (success, HTTP error, network error, timeout, cancellation, JSON-parse error), the timeout-vs-caller-abort race is handled with a first-writer-wins guard so simultaneous timeout+cancel doesn't produce ambiguous error typing, and the test suite genuinely simulates a hanging request via a fake-timer harness rather than just asserting on mocked returns (it also asserts zero pending timers and zero leaked listeners after completion, which is the part most implementations skip). All 11 tests in api.test.mjs pass, and every existing caller of the touched wrapper functions still works with the new optional options param.
But alongside the new apiFetch, three existing wrapper functions (getCVESummary, getPlaybook, getScan) had their catch blocks narrowed from "catch anything, fall back gracefully" to "only catch ApiHttpError, rethrow everything else" - and two of those have real call sites that were never updated for the new thrown error types, which is a functional regression in exactly the scenario (long-running operation, imperfect network) this PR is meant to make more robust. Left inline comments on the two consequential ones.
Suggested fix direction: either broaden the catch back to cover transient failures (e.g. ApiHttpError | ApiNetworkError, letting only ApiCancellationError propagate since that's genuinely caller-initiated) or update the Header.jsx poll loop and DetailedScan.jsx's selectFinding to handle the new error types explicitly. Also worth noting for later: no component actually passes an AbortController/signal into any api.* call yet, so while apiFetch now correctly composes a caller signal with its internal timeout, nothing aborts in-flight requests on unmount/re-trigger today - the race condition the PR title references isn't fixed at any call site yet, just made possible. Not blocking, just flagging so it isn't mistaken for done.
| getScan: async (scanId, options = {}) => { | ||
| try { return await apiFetch(`/scans/${scanId}`, options); } | ||
| catch (err) { | ||
| if (!(err instanceof ApiHttpError)) throw err; |
There was a problem hiding this comment.
This used to be a bare catch { ... } that fell back to listing scans on any failure. Now it only falls back on ApiHttpError and rethrows everything else - including the new ApiTimeoutError/ApiNetworkError.
Header.jsx's executeScan polls this in a loop (for (let i = 0; i < 75; i++) { ...; const scan = await api.getScan(scanId); ... }, ~5 minutes at 4s intervals) with a single try/catch around the whole loop. A single transient network blip or timeout on any one poll now throws straight out of the loop and ends polling entirely - the user gets a "Scan failed" toast even though the backend scan is still running to completion. Before this PR there was no timeout and any transient error fell back to /scans and let the loop continue.
Given getScan doesn't override timeoutMs, it also now inherits the new default 30s timeout per call, so this isn't just a network-blip edge case - a single slow response during the 5-minute poll is enough to trigger it.
| catch (err) { | ||
| if (err instanceof ApiHttpError) { | ||
| return { portalSteps: [], cliCommands: [], validationSteps: [], references: [] }; | ||
| } |
There was a problem hiding this comment.
Same narrowing as getScan below. DetailedScan.jsx's selectFinding calls await api.getPlaybook(f.id) with no try/catch, on both mount and click, so a timeout/network error here becomes an unhandled promise rejection with no fallback UI - actually worse than the pre-PR behavior (which returned the empty-arrays fallback) for exactly the failure modes this PR targets.
|
@SHAURYAKSHARMA24, its been a week since @parthrohit22 has requested changes and has not been addressed, so please give it a look. |
Signed-off-by: Shaurya K Sharma <shauryaksharma24@gmail.com>
Signed-off-by: Shaurya K Sharma <shauryaksharma24@gmail.com>
fc174d0 to
a9d8339
Compare
|
Addressed the transport-error regressions from review. Scan polling no longer treats a transient timeout/network failure as backend scan failure, while explicit caller cancellation still propagates. The playbook and CVE-summary paths preserve graceful handling for HTTP/network/timeout failures without swallowing caller-initiated cancellation. Added wrapper and polling regression coverage, rebased onto current dev, and reran the frontend API, polling, page-data, severity, a11y/i18n, lint, build, and website tests. All CI checks are green on the updated head. Ready for re-review. |
4 participants
What does this PR do?
Adds a shared 30-second default timeout and caller cancellation support to the frontend API client without introducing automatic retries.
The request layer composes an internal
AbortControllerwith an optional caller-providedAbortSignal, clears timeout handles and caller listeners infinally, and accepts a per-calltimeoutMsoverride (nulldisables the internal timeout). Public API wrappers forward request options, including custom headers.Timeouts, caller cancellation, HTTP responses, and network failures use distinct exported error classes and codes:
ApiHttpErrorApiNetworkErrorApiTimeoutErrorApiCancellationErrorScan polling treats individual timeout/network poll failures as transient and continues toward a later terminal backend status.
getScan()retains the direct-endpoint HTTP compatibility fallback to/scans, while avoiding a second request after a timeout/network failure. Explicit caller cancellation still propagates and stops polling when a signal is supplied.Optional playbook and CVE-summary data preserve graceful fallbacks for HTTP/network/timeout failures while allowing explicit caller cancellation to propagate. Current components do not yet create AbortSignals; this PR provides and verifies the request-layer capability and polling support.
No automatic retry policy was added. State-changing requests such as scan triggers are attempted once, and caller options cannot override the authoritative POST method/body.
Type of change
Testing
Executed from
frontend/:node src/utils/api.test.mjs— 25 passednode src/utils/scanPolling.test.mjs— 4 passednode src/utils/aiApi.test.mjs— 9 passednode src/hooks/usePageData.test.mjs— 8 passednpm run test:severity— 5 passednpm run test:a11y— passednpm run test:i18n— passednpm run lint— passed with zero warningsnpm run build— passedExecuted from the repository root:
node website/test_toEmbedUrl.mjs— 15 passedCoverage includes success, HTTP/network/parse failures, timeout and caller-abort classification, already-aborted signals, caller-vs-timeout races, timeout overrides/disable/validation, timer and listener cleanup, auth/custom headers, scan HTTP fallback, transient scan polling recovery, terminal failed scans, optional-data fallbacks, cancellation propagation, and single-attempt POST safety.
Related issue
Closes #282
Checklist
Signed-off-bytrailer